pydrake/doc: Add new style, header, footer #13983
Conversation
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge" (waiting on @jwnimmer-tri)
bindings/pydrake/doc/_templates/layout.html, line 16 at r1 (raw file):
<ul class="menu__list"> <li class="menu__list__item menu__list__item__link button--text"> <a href="https://drake.mit.edu/quickstart.html" class="menu__list__item__link button--text">Quickstart</a>
There is no such link target (quickstart.html). Should this be installation.html?
jwnimmer-tri
force-pushed
the
doc-pydrake-style
branch
2 times, most recently
from
40a9370 to
6a05189
Compare
There was a problem hiding this comment.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge"
bindings/pydrake/doc/_templates/layout.html, line 16 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
There is no such link target (
quickstart.html). Should this beinstallation.html?
In a forthcoming PR from the contractors to replace the landing page, there will be a quickstart.html file. But I guess for now, I'll link to the old filename and we'll retarget this once the other PR lands.
|
+@EricCousineau-TRI are you available to feature review with high priority? \CC @jamiesnape FYI |
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers (waiting on @EricCousineau-TRI and @jwnimmer-tri)
a discussion (no related file):
Working to confirm copyright of the github svgs.
There was a problem hiding this comment.
We are building upon the ReadTheDocs theme?
Reviewable status: 1 unresolved discussion, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers (waiting on @EricCousineau-TRI and @jwnimmer-tri)
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Working to confirm copyright of the github svgs.
We need to satisfy the trademark terms too, I think, i.e., satisfy https://github.com/logos.
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers (waiting on @EricCousineau-TRI and @jwnimmer-tri)
bindings/pydrake/doc/_static/css/custom.css, line 201 at r2 (raw file):
kbd, samp { font-family: monospace, monospace;
This is not going to work with respect to unicode notation.
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers (waiting on @EricCousineau-TRI and @jwnimmer-tri)
bindings/pydrake/doc/_static/css/custom.css, line 1 at r2 (raw file):
@import url('https://fonts.googleapis.com/css2?family=Work+Sans:wght@300;400;600;700;800&display=swap');
BTW some people object to Google Fonts for privacy reasons.
There was a problem hiding this comment.
I have no idea, not my project. I'm just the bits shepherd. I can help redirect questions to the responsible parties if you can elaborate on the question.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers (waiting on @EricCousineau-TRI, @jamiesnape, and @jwnimmer-tri)
a discussion (no related file):
Previously, jamiesnape (Jamie Snape) wrote…
We need to satisfy the trademark terms too, I think, i.e., satisfy https://github.com/logos.
Yup, I just got confirmation that these are indeed the mark files from that site.
My plan is to move them into third_party and add a README.
I'll also see what's possible in terms of keeping the README or alike as part of the site deployment?
bindings/pydrake/doc/_static/css/custom.css, line 201 at r2 (raw file):
Previously, jamiesnape (Jamie Snape) wrote…
This is not going to work with respect to unicode notation.
The https://jwnimmer-tri.github.io/RobotLocomotion.github.io/pydrake/pydrake.multibody.plant.html#pydrake.multibody.plant.MultibodyPlant_.MultibodyPlant_[float].CalcJacobianSpatialVelocity for example looks find to me. Are you saying for macOS we need to adjust this?
There was a problem hiding this comment.
So technically we are. All the stylesheets and layouts are inherited from that theme. However, we override much, if not all, of that, and ideally we would not need the extra apt package installing.
Not to worry, I will look into it once it is deployed and see if the base theme is effectively redundant.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers (waiting on @EricCousineau-TRI and @jwnimmer-tri)
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Yup, I just got confirmation that these are indeed the mark files from that site.
My plan is to move them into third_party and add a README.
I'll also see what's possible in terms of keeping the README or alike as part of the site deployment?
Ideally we would write a NOTICE file into the root of the repo, but that would correspond to a different target than this (though maybe the same assets are going to be used for both Sphinx sites?)
bindings/pydrake/doc/_static/css/custom.css, line 201 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
The https://jwnimmer-tri.github.io/RobotLocomotion.github.io/pydrake/pydrake.multibody.plant.html#pydrake.multibody.plant.MultibodyPlant_.MultibodyPlant_[float].CalcJacobianSpatialVelocity for example looks find to me. Are you saying for macOS we need to adjust this?
It is the exact of opposite of the fix I just put in for Doxygen, so yes.
The default monospace font on macOS does not have the extended character set. You are going to get different fonts on each platform (not just here, elsewhere in this style) which is going to be difficult for cross platform development with the number of unicode characters that we use.
We have somewhat unusual requirements beyond aesthetics here, unfortunately.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers (waiting on @EricCousineau-TRI and @jwnimmer-tri)
a discussion (no related file):
... root of the repo ...
To be clear, do you mean the pages repo (https://github.com/RobotLocomotion/RobotLocomotion.github.io)?
... maybe the same assets are going to be used for both Sphinx sites ...
I haven't seen any files yet for the main sphinx site's changes, but my guess would be yes.
bindings/pydrake/doc/_static/css/custom.css, line 201 at r2 (raw file):
Previously, jamiesnape (Jamie Snape) wrote…
It is the exact of opposite of the fix I just put in for Doxygen, so yes.
The default
monospacefont on macOS does not have the extended character set. You are going to get different fonts on each platform (not just here, elsewhere in this style) which is going to be difficult for cross platform development with the number of unicode characters that we use.We have somewhat unusual requirements beyond aesthetics here, unfortunately.
Help me understand how to resolve this. Is it something where we can tweak this CSS a little bit and get macOS working again? Or is it a substantial enough change where we need to engage with the contractors to find a middle ground and show them how to test (i.e., which methods to browse) on macOS?
jwnimmer-tri
force-pushed
the
doc-pydrake-style
branch
from
6a05189 to
88c5481
Compare
There was a problem hiding this comment.
Let me know if there is an email thread you'd like me to direct my comments to.
Reviewed 8 of 9 files at r1, 1 of 1 files at r2, 5 of 5 files at r3.
Reviewable status: 7 unresolved discussions, needs at least two assigned reviewers (waiting on @jwnimmer-tri)
bindings/pydrake/doc/_static/drake-logo-white.svg, line 0 at r2 (raw file):
nit It's rather hard to (easily) preview, given that GitHub's preview doesn't seem to render well?
It's preview-able with Eye of Gnome (eog); however, in Inkscape, it requires changing the background (resaving in Inkscape with inkscape:pagecheckerboard="true"):
Is it possible to request that the contractors always send us SVGs saved with Inkscape? (at a minimum, so local tools are both OK? Otherwise, perhaps we figure out why GitHub preview is buggy?)
Let me know if you'd like me to make this request in a separate email thread.
(Feel free to dismiss for now)
bindings/pydrake/doc/_static/css/custom.css, line 3 at r2 (raw file):
@import url('https://fonts.googleapis.com/css2?family=Work+Sans:wght@300;400;600;700;800&display=swap'); @charset "UTF-8";
Is it possible for us to request the contracts describe the process they followed to generate this stylesheet?
It's unclear if this is was automatically generated with direct tweaks, or by another process.
If there is a source used to generate it, it would be nice to have those source files.
The reference to Bootstrap leads me to believe it was used, but there's no real traceability here.
bindings/pydrake/doc/_static/css/custom.css, line 611 at r2 (raw file):
.wy-nav-side .icon.icon-home { display: none;
nit Inconsistent use of spaces vs. tabs.
bindings/pydrake/doc/_static/css/custom.css, line 969 at r2 (raw file):
.header--main .header__menu { /*
nit Commented out block, should be deleted?
bindings/pydrake/doc/_static/css/custom.css, line 1144 at r2 (raw file):
border-top: 2px solid #EB0A1E; background-color: black;
nit Trailing space.
bindings/pydrake/doc/_templates/layout.html, line 71 at r2 (raw file):
</div> <div class="footer__copyright wrap"> <span class="footnote">© 2020 The Drake Development Team</span>
nit Should we have a more explicit copyright? e.g. MIT or TRI? Or is this OK for now?
There was a problem hiding this comment.
Reviewable status: 7 unresolved discussions, needs at least two assigned reviewers (waiting on @EricCousineau-TRI and @jwnimmer-tri)
bindings/pydrake/doc/_static/css/custom.css, line 3 at r2 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Is it possible for us to request the contracts describe the process they followed to generate this stylesheet?
It's unclear if this is was automatically generated with direct tweaks, or by another process.
If there is a source used to generate it, it would be nice to have those source files.The reference to Bootstrap leads me to believe it was used, but there's no real traceability here.
Is this question about copyright or ...? If the former, I can double-check to answer that narrow question. Or else, what is the defect?
bindings/pydrake/doc/_templates/layout.html, line 71 at r2 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit Should we have a more explicit copyright? e.g. MIT or TRI? Or is this OK for now?
I am content with this line. The contractors actually used a different one (see r1..r2) and this is what I replaced it with. Relates #1805.
There was a problem hiding this comment.
Reviewable status: 7 unresolved discussions, needs at least two assigned reviewers (waiting on @EricCousineau-TRI and @jwnimmer-tri)
bindings/pydrake/doc/_static/css/custom.css, line 3 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Is this question about copyright or ...? If the former, I can double-check to answer that narrow question. Or else, what is the defect?
Defect is maintainability. If we need to tweak, are we able to do it ourselves, or will we need to get the contractors to reuse the (hidden?) artifacts or rediscover the process?
There was a problem hiding this comment.
Reviewable status: 7 unresolved discussions, needs at least two assigned reviewers (waiting on @EricCousineau-TRI and @jwnimmer-tri)
bindings/pydrake/doc/_static/css/custom.css, line 3 at r2 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Defect is maintainability. If we need to tweak, are we able to do it ourselves, or will we need to get the contractors to reuse the (hidden?) artifacts or rediscover the process?
Ah, you were thinking that this might be a machine-generated file, and we need access to the original in order to regenerate it? That's not the fase. This file is the primary representation. We can PR to it directly in the future.
There was a problem hiding this comment.
Reviewable status: 7 unresolved discussions, needs at least two assigned reviewers (waiting on @EricCousineau-TRI and @jwnimmer-tri)
bindings/pydrake/doc/_static/css/custom.css, line 3 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Ah, you were thinking that this might be a machine-generated file, and we need access to the original in order to regenerate it? That's not the fase. This file is the primary representation. We can PR to it directly in the future.
s/fase/case/
There was a problem hiding this comment.
Reviewable status: 7 unresolved discussions, needs at least two assigned reviewers (waiting on @EricCousineau-TRI and @jwnimmer-tri)
bindings/pydrake/doc/_static/css/custom.css, line 3 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
s/fase/case/
Gotcha. Then the defect now is, why is Bootstrap mentioned at all?
jwnimmer-tri
force-pushed
the
doc-pydrake-style
branch
from
88c5481 to
8f8026a
Compare
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, needs at least two assigned reviewers (waiting on @EricCousineau-TRI and @jwnimmer-tri)
bindings/pydrake/doc/_static/drake-logo-white.svg, line at r2 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit It's rather hard to (easily) preview, given that GitHub's preview doesn't seem to render well?
It's preview-able with Eye of Gnome (eog); however, in Inkscape, it requires changing the background (resaving in Inkscape with
inkscape:pagecheckerboard="true"):
Is it possible to request that the contractors always send us SVGs saved with Inkscape? (at a minimum, so local tools are both OK? Otherwise, perhaps we figure out why GitHub preview is buggy?)
Let me know if you'd like me to make this request in a separate email thread.(Feel free to dismiss for now)
OK This seems like something we can deal with offline, and in any case for a file that will not be changed regularly (if at all), to me it doesn't seem worth worrying at all about the ease of the pull request image preview workflow? I provided a link to the staging site; that's the best place to see how things look.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4.
Reviewable status: 2 unresolved discussions, needs at least two assigned reviewers (waiting on @jwnimmer-tri)
bindings/pydrake/doc/_static/drake-logo-white.svg, line at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
OK This seems like something we can deal with offline, and in any case for a file that will not be changed regularly (if at all), to me it doesn't seem worth worrying at all about the ease of the pull request image preview workflow? I provided a link to the staging site; that's the best place to see how things look.
OK Sounds good!
bindings/pydrake/doc/_templates/layout.html, line 71 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
I am content with this line. The contractors actually used a different one (see r1..r2) and this is what I replaced it with. Relates #1805.
OK SGTM. Thanks for the explanation!
jwnimmer-tri
force-pushed
the
doc-pydrake-style
branch
from
8f8026a to
35f075f
Compare
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, needs at least two assigned reviewers (waiting on @EricCousineau-TRI and @jwnimmer-tri)
bindings/pydrake/doc/_static/css/custom.css, line 3 at r2 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Gotcha. Then the defect now is, why is Bootstrap mentioned at all?
Done (removed).
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, needs at least two assigned reviewers (waiting on @EricCousineau-TRI and @jwnimmer-tri)
a discussion (no related file):
To be clear, do you mean the pages repo (https://github.com/RobotLocomotion/RobotLocomotion.github.io)?
Yes.
bindings/pydrake/doc/_static/css/custom.css, line 201 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Help me understand how to resolve this. Is it something where we can tweak this CSS a little bit and get macOS working again? Or is it a substantial enough change where we need to engage with the contractors to find a middle ground and show them how to test (i.e., which methods to browse) on macOS?
They basically need a variation of what was already there.
The font-family of the elements that hold the generated content all need to resolve to fonts with an extended character set (does not have to be Deja Vu). There are no guarantees the generic font-families have that on any platform (though Ubuntu is fine with the common browsers).
They can eyeball it if they like, but as we saw last week, there are plenty of elements that we may use in future, but have not yet.
Can we fix it? Yes, but it might change the aesthetic, so the original authors are the best to do so. monospace is easy enough, but there are quite a few other font-families to look into.
bindings/pydrake/doc/_templates/layout.html, line 39 at r5 (raw file):
</li> <li class="menu__list__item menu__list__item__link button--text github--link"> <a href="https://github.com/RobotLocomotion/drake" class="menu__list__item__link button--text">Github <img src="_static/github-white.svg"></a>
BTW GitHub
bindings/pydrake/doc/_templates/layout.html, line 64 at r5 (raw file):
<li class="menu__list__item menu__list__item__link button--text github--link"> <a href="https://github.com/RobotLocomotion/drake" class="menu__list__item__link button--text"> Github <img src="_static/github.svg">
BTW GitHub
The css, images, header, and footer text were created by a TRI contractor that assigned all rights to TRI. The only part I wrote myself was the sphinx and build system integration. Co-authored-by: TRI contractors <[email protected]>
jwnimmer-tri
force-pushed
the
doc-pydrake-style
branch
from
35f075f to
c25db70
Compare
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, needs at least two assigned reviewers (waiting on @EricCousineau-TRI and @jwnimmer-tri)
bindings/pydrake/doc/_static/css/custom.css, line 201 at r2 (raw file):
Previously, jamiesnape (Jamie Snape) wrote…
They basically need a variation of what was already there.
The font-family of the elements that hold the generated content all need to resolve to fonts with an extended character set (does not have to be Deja Vu). There are no guarantees the generic font-families have that on any platform (though Ubuntu is fine with the common browsers).
They can eyeball it if they like, but as we saw last week, there are plenty of elements that we may use in future, but have not yet.
Can we fix it? Yes, but it might change the aesthetic, so the original authors are the best to do so.
monospaceis easy enough, but there are quite a few other font-families to look into.
=> https://app.slack.com/client/T0JNB2DS4/C01879RNKQW/thread/C01879RNKQW-1598984546.010200
There was a problem hiding this comment.
Reviewed 1 of 1 files at r5, 1 of 1 files at r6.
Reviewable status: 1 unresolved discussion, needs at least two assigned reviewers (waiting on @jwnimmer-tri)
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI and @jamiesnape)
bindings/pydrake/doc/_static/css/custom.css, line 201 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
=> https://app.slack.com/client/T0JNB2DS4/C01879RNKQW/thread/C01879RNKQW-1598984546.010200
Done (claimed).
|
@jamiesnape PTAL |
There was a problem hiding this comment.
I not 100% confident in this, at least in the way I was with the version in master now, but tracing through the most common cascades, I think we are ok, so
Reviewed 4 of 9 files at r1, 5 of 5 files at r3, 1 of 1 files at r6, 1 of 1 files at r7.
Reviewable status: 1 unresolved discussion, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @jwnimmer-tri)
bindings/pydrake/doc/_static/css/custom.css, line 201 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Done (claimed).
Looks like the whole block needs removing actually. If you want to keep it, it would need to be font-family: 'DejaVu Sans Mono' monospace; What is here now does not make sense.
bindings/pydrake/doc/_static/css/custom.css, line 2 at r7 (raw file):
/* s3://drake-web/fonts/stylesheet.css */ @import url('https://dmn3132s4wr9m.cloudfront.net/fonts/stylesheet.css');
I think we still need this since we are still using DejaVu fonts.
|
Just for the record, what's the next action item on this PR? (licensing, or something else?) |
|
This is on "do not merge hold" because we don't want to change the pydrake style prior to changing the main site style. Once the main site is close to ready, we'll finish this one up. |
|
The artifacts introduced here should be checked; if they are possibly derivatives of the original Jekyll website setup, we may want to purge them. Context: |
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @jwnimmer-tri)
bindings/pydrake/doc/_static/css/custom.css, line 87 at r7 (raw file):
*, *::before,
This appears to be derivative.
@jwnimmer-tri Can we close this PR and delete the branch?
|
Yup! |
Preview available at: https://jwnimmer-tri.github.io/RobotLocomotion.github.io/pydrake/index.html
The css, images, header, and footer text were created by a TRI contractor that assigned all rights to TRI. The only part I wrote myself was the sphinx and build system integration.
Relates #13832.
This change is